Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds client-side routing functionality to a Respo-based web application, implementing a Single Page Application (SPA) architecture with browser history integration.
Key Changes:
- Introduces a router module using the
ruled-routerlibrary with route definitions, navigation helpers, and browser history management - Adds page components that render different views based on the current route
- Integrates router state into the application store with new actions for route changes and restoration
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/router.rs | New module defining routes (Home, Counter, CounterDetail), navigation functions, history management, and popstate event handling |
| src/pages.rs | New module with navigation links component and route-based page rendering logic |
| src/store.rs | Adds router state field and RouteChange/RouteRestore actions; modernizes formatting |
| src/main.rs | Integrates router initialization, sets up popstate listener for browser navigation, prioritizes URL over stored state |
| Cargo.toml | Adds ruled-router dependencies and new web-sys features (History, Location, PopStateEvent) |
| Cargo.lock | Updates dependency versions (respo, wasm-bindgen, web-sys, serde) |
| Trunk.toml | Changes public_url from relative to absolute path for SPA routing; adds serve configuration |
| src/counter.rs | Modernizes string formatting to use inline variable interpolation |
| .gitignore | Adds .DS_Store to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn push_history_state(path: &str) { | ||
| if let Some(window) = web_sys::window() { | ||
| if let Ok(history) = window.history() { | ||
| let _ = history.push_state_with_url(&JsValue::NULL, "", Some(path)); |
There was a problem hiding this comment.
Errors from push_state_with_url() are silently ignored. If the history API call fails, navigation will be incomplete (URL won't change but app state will). Consider propagating or logging this error to make debugging navigation issues easier.
| push_history_state(&path); | ||
| dispatch.run(ActionOp::RouteChange(route)) |
There was a problem hiding this comment.
The function pushes history state before dispatching the action. If dispatch.run() fails, the browser history will be updated but the application state won't, causing a mismatch between URL and app state. Consider either: (1) dispatching first and only pushing history on success, or (2) handling the error by popping the history state if dispatch fails.
| push_history_state(&path); | |
| dispatch.run(ActionOp::RouteChange(route)) | |
| dispatch.run(ActionOp::RouteChange(route)).map(|_| { | |
| push_history_state(&path); | |
| }) |
|
|
||
| /// Home page component | ||
| fn comp_home_page(home_route: &router::HomeRoute) -> Result<RespoElement<ActionOp>, String> { | ||
| let welcome_msg = home_route.query.welcome.clone().unwrap_or_else(|| "Welcome Home!".to_string()); |
There was a problem hiding this comment.
The welcome query parameter is cloned unnecessarily. Consider using .as_deref().unwrap_or("Welcome Home!") to avoid cloning the string when it exists, improving performance for this hot path.
| let welcome_msg = home_route.query.welcome.clone().unwrap_or_else(|| "Welcome Home!".to_string()); | |
| let welcome_msg = home_route.query.welcome.as_deref().unwrap_or("Welcome Home!"); |
| // ============================================================================ | ||
|
|
||
| /// Top-level route matcher (enum for matching different routes) | ||
| /// Note: Order matters! More specific routes should come first. |
There was a problem hiding this comment.
The comment states "Order matters! More specific routes should come first." However, Counter (with pattern /counter) is listed before Home (pattern /). Since /counter is more specific than /, this ordering is actually correct. Consider revising the comment to be more explicit: "Order matters! List more specific routes (like /counter) before general routes (like /) to ensure proper matching."
| /// Note: Order matters! More specific routes should come first. | |
| /// Order matters! List more specific routes (like `/counter`) before general routes (like `/`) to ensure proper matching. |
| // Leak the closure to keep it alive for the lifetime of the app | ||
| callback.forget(); |
There was a problem hiding this comment.
The closure is intentionally leaked to keep it alive, which prevents proper cleanup. Consider using Rc<RefCell<Option<Closure>>> or a similar pattern to store the closure so it can be properly managed and potentially cleaned up. Memory leaks can accumulate if the router is re-initialized multiple times.
| web_sys::window() | ||
| .and_then(|w| { | ||
| let pathname = w.location().pathname().ok()?; | ||
| let search = w.location().search().unwrap_or_default(); |
There was a problem hiding this comment.
If w.location().search() fails (unlikely but possible), it uses an empty default but doesn't log the failure. While the fallback is reasonable, consider logging this for debugging purposes since it indicates a browser API issue.
| }) as Box<dyn Fn(_)>); | ||
|
|
||
| if let Some(window) = web_sys::window() { | ||
| let _ = window.add_event_listener_with_callback("popstate", callback.as_ref().unchecked_ref()); |
There was a problem hiding this comment.
Errors from add_event_listener_with_callback() are silently ignored. If adding the event listener fails, browser back/forward navigation won't work, but there will be no indication of this failure. Consider logging or propagating this error.
|
|
||
| app.try_load_storage().expect("load storage"); | ||
| // Load storage (may contain outdated router state) | ||
| let _ = app.try_load_storage(); |
There was a problem hiding this comment.
The error from try_load_storage() is silently discarded. If loading storage fails, this could indicate a problem (corrupted data, incompatible format). Consider logging the error or providing user feedback when storage loading fails.
| ActionOp::Noop => {} | ||
| ActionOp::Increment => self.counted += 1, | ||
| ActionOp::Decrement => self.counted -= 1, | ||
| ActionOp::StatesChange(a) => self.update_states(a), |
There was a problem hiding this comment.
Both RouteChange and RouteRestore update the router state identically, but they serve different purposes (one pushes history, one doesn't). This logic conflates the distinction. Consider adding a comment explaining that the history manipulation happens before dispatch in navigate_to(), or track this separately if the distinction matters for debugging or undo/redo functionality.
| ActionOp::StatesChange(a) => self.update_states(a), | |
| ActionOp::StatesChange(a) => self.update_states(a), | |
| // Both RouteChange and RouteRestore update the router state identically. | |
| // The distinction (history push or not) is handled before dispatching this action, | |
| // e.g., in the navigate_to() function. If tracking the distinction is needed for | |
| // debugging or undo/redo, consider storing the last action type. |
| { | ||
| let store = app.store.clone(); | ||
| setup_router_listener(move |route| { | ||
| let _ = store.borrow_mut().update(ActionOp::RouteRestore(route)); |
There was a problem hiding this comment.
The error from store.borrow_mut().update() is silently discarded. If the route update fails during browser back/forward navigation, the UI state will become inconsistent with the URL. Consider logging this error or displaying it to the user.
|
conflicts |
|
my bad. now rebased. |
most of the code powered by AI.